Cursor and zoom polish, playback prefetch fixes, and platform onboarding tweaks#1686
Cursor and zoom polish, playback prefetch fixes, and platform onboarding tweaks#1686richiemcilroy merged 18 commits intomainfrom
Conversation
…efaults Made-with: Cursor
…prefetch Made-with: Cursor
… cap Made-with: Cursor
…refetch Made-with: Cursor
…r default Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…where Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…owlist Made-with: Cursor
| .recv_timeout(Duration::from_millis(wait_ms)) | ||
| { | ||
| Ok(p) => Some(p), | ||
| Err(std_mpsc::RecvTimeoutError::Timeout) => prefetch_rx.try_recv().ok(), |
There was a problem hiding this comment.
The timeout path currently does prefetch_rx.try_recv().ok(), which treats Disconnected the same as Empty and can lead to endless skipping + frame requests after the prefetch thread exits. Handling TryRecvError::Disconnected explicitly avoids that.
| Err(std_mpsc::RecvTimeoutError::Timeout) => prefetch_rx.try_recv().ok(), | |
| Err(std_mpsc::RecvTimeoutError::Timeout) => match prefetch_rx.try_recv() { | |
| Ok(p) => Some(p), | |
| Err(std_mpsc::TryRecvError::Empty) => None, | |
| Err(std_mpsc::TryRecvError::Disconnected) => { | |
| break 'playback; | |
| } | |
| }, |
| .map(|t| t.zoom_segments.as_slice()) | ||
| .unwrap_or(&[]), | ||
| ); | ||
| let precomputed_cursor = &cursor_timelines[segment_index as usize]; |
There was a problem hiding this comment.
cursor_timelines[segment_index as usize] will panic if a bad segment_index ever slips through (and zoom_interpolators.get(..) already accounts for out-of-range). Using .get(..) here keeps playback resilient.
| let precomputed_cursor = &cursor_timelines[segment_index as usize]; | |
| let Some(precomputed_cursor) = cursor_timelines.get(segment_index as usize) else { | |
| break 'playback; | |
| }; |
| let min_buffered = prefetch_buffer.iter().map(|p| p.frame_number).min(); | ||
| if let Some(next_available_frame) = min_buffered | ||
| && next_available_frame > frame_number | ||
| // IMPORTANT: Do NOT send frame_request_tx from these skip paths. |
There was a problem hiding this comment.
Heads up: this repo has a strict no-code-comments rule (no // / /* */). These newly added inline comments should be removed (the intent can be captured via naming/extraction instead).
| *last_sent_frame.borrow_mut() = Some(data.clone()); | ||
| let _ = req.sender.send(data.to_decoded_frame()); | ||
| } else { | ||
| // IMPORTANT: When the decoder advances past a requested frame |
There was a problem hiding this comment.
Same note here: the new // IMPORTANT: block violates the repo’s no-code-comments rule. Suggest removing the inline commentary and relying on naming/structure (or logging) to preserve the intent.
| let data = cached.data().clone(); | ||
| let _ = req.sender.send(data.to_decoded_frame()); | ||
| } else { | ||
| // See the matching comment in the decode-loop fallback above for full |
There was a problem hiding this comment.
This additional // block also needs to go per the no-code-comments rule.
| precomputed_cursor: Option<std::sync::Arc<PrecomputedCursorTimeline>>, | ||
| cursor_smoothing: Option<SpringMassDamperSimulationConfig>, | ||
| click_spring: ClickSpringConfig, | ||
| _precomputed_cursor: Option<std::sync::Arc<PrecomputedCursorTimeline>>, |
There was a problem hiding this comment.
_precomputed_cursor, _cursor_smoothing, and _click_spring are now stored but unused. If they’re no longer part of the algorithm, consider removing them (and the corresponding ctor params) so callers don’t pay compute/setup cost and future readers don’t assume they affect behavior.
| _actual_cursor: Option<Coord<RawDisplayUVSpace>>, | ||
| ) -> Self { | ||
| let is_auto_mode = matches!(segment.mode, cap_project::ZoomMode::Auto); |
There was a problem hiding this comment.
Unused public parameter in
from_segment_with_cursor_constraint
_actual_cursor is now entirely unused inside the function — the whole calculate_zoom_and_center_for_cursor branch it previously fed was removed. As a pub function its signature is part of the crate's interface, so leaving a permanently ignored parameter can confuse callers about what they're expected to provide.
If the parameter has no planned future use, consider removing it from the signature and updating all call sites. If it's intentionally kept as a hook for future work, a // TODO: comment explains the intent.
pub fn from_segment_with_cursor_constraint(
segment: &ZoomSegment,
zoom_focus: Coord<RawDisplayUVSpace>,
- _actual_cursor: Option<Coord<RawDisplayUVSpace>>,
) -> Self {Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/rendering/src/zoom.rs
Line: 62-64
Comment:
**Unused public parameter in `from_segment_with_cursor_constraint`**
`_actual_cursor` is now entirely unused inside the function — the whole `calculate_zoom_and_center_for_cursor` branch it previously fed was removed. As a `pub` function its signature is part of the crate's interface, so leaving a permanently ignored parameter can confuse callers about what they're expected to provide.
If the parameter has no planned future use, consider removing it from the signature and updating all call sites. If it's intentionally kept as a hook for future work, a `// TODO:` comment explains the intent.
```rust
pub fn from_segment_with_cursor_constraint(
segment: &ZoomSegment,
zoom_focus: Coord<RawDisplayUVSpace>,
- _actual_cursor: Option<Coord<RawDisplayUVSpace>>,
) -> Self {
```
How can I resolve this? If you propose a fix, please make it concise.| [package] | ||
| name = "cap-desktop" | ||
| version = "0.4.8" | ||
| version = "0.4.81" |
There was a problem hiding this comment.
Unusual version bump
0.4.8 → 0.4.81
Semver patch increments conventionally go 0.4.8 → 0.4.9. Jumping to 0.4.81 (skipping 72 patch levels) is valid semver but looks like either a typo (0.4.9 was intended) or a deliberate "sub-patch" scheme. If this is intentional (e.g., rapid hotfix numbering), a comment or convention note in the release process would prevent future confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/Cargo.toml
Line: 3
Comment:
**Unusual version bump `0.4.8` → `0.4.81`**
Semver patch increments conventionally go `0.4.8 → 0.4.9`. Jumping to `0.4.81` (skipping 72 patch levels) is valid semver but looks like either a typo (`0.4.9` was intended) or a deliberate "sub-patch" scheme. If this is intentional (e.g., rapid hotfix numbering), a comment or convention note in the release process would prevent future confusion.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Tightens desktop recording and editing around cursor motion, automatic zoom, and preview playback, and fixes a few cross-platform UX issues.
Greptile Summary
This PR delivers a broad polish pass across desktop recording and editing: it fixes playback prefetch regressions (cascading decode resets, overly-restrictive frame fallback), refines cursor and zoom interpolation (unclamped spring positions, gentle cursor-visibility enforcement, new
Smooth/Fastanimation presets), improves cross-platform onboarding (skipping macOS permissions on Windows/Linux), and tidies several UX details (resize handle, hex color inputEntercommit,MainWindowHelpButtonplacement, settings window minimum bounds).\n\nKey changes:\n- Playback prefetch (playback.rs,avassetreader.rs): Cursor timelines and zoom interpolators are now pre-built outside the frame loop instead of constructed per-frame; buffer-gap skip paths no longer sendframe_request_tx(which was resetting the prefetch pipeline); frame fallback unified to backward-preferred with 90-frame max distance.\n- Cursor spring (cursor_interpolation.rs,lib.rs,cursor.rs): Removed 0–1 clamping from the smoothed timeline; out-of-range positions are now filtered at render time, eliminating the artificial "wall" that distorted spring dynamics near screen edges.\n- Zoom interpolation (zoom_focus_interpolation.rs,zoom.rs): Focus target now uses raw cursor position with a built-in lag-clamping constraint; zoom viewport shift replaced dynamic zoom-reduction with a gentler viewport pan that keeps the cursor within a 3% margin.\n-CursorAnimationStyle(configuration.rs): AddsSmoothandFastvariants; notably removes\"fast\"as a serde alias forMellow— any project JSON storing that string will now deserialize toFast(different physics) rather thanMellow.\n- Onboarding (onboarding.tsx): Permissions step is now macOS-only; non-macOS users havepermsGrantedset totrueon effect mount and skip straight to step 1.\n- UX polish: Settings window minimum bounds enforced on re-focus; resize handle doubled to 16 px with 3-dot grip; hex color inputs commit onEnter;MainWindowHelpButtonshown on all platforms.Confidence Score: 4/5
Safe to merge; no runtime-critical bugs found. One P1 serde migration concern has very low real-world exposure since 'fast' was never surfaced in the UI.
The playback prefetch fixes are well-reasoned and thoroughly commented. Cursor/zoom interpolation changes are logically consistent. Cross-platform onboarding gating is correct. Minor deductions for: the 'fast' serde alias repurposing (P1 but low real-world risk), three dead-storage fields kept in ZoomFocusInterpolator, and an unused public parameter in from_segment_with_cursor_constraint. None of these block shipping.
crates/project/src/configuration.rs (serde alias migration), crates/rendering/src/zoom_focus_interpolation.rs (dead fields), crates/rendering/src/zoom.rs (unused public param)
Important Files Changed
Sequence Diagram
sequenceDiagram participant PL as Playback loop participant CT as CursorTimeline[] participant ZI as ZoomInterpolator[] participant PFQ as Prefetch queue participant RX as prefetch_rx participant R as Renderer note over PL,ZI: Pre-built once (or on project change) PL->>CT: build_cursor_timelines(project) PL->>ZI: build_zoom_interpolators(project, cursor_timelines) loop Each frame PL->>PFQ: drain available frames (try_recv) alt frame in buffer PL->>ZI: ensure_precomputed_until(frame_time) PL->>R: render_frame(frames, uniforms, precomputed_cursor) else buffer empty PL->>RX: recv_timeout(8-20 ms) RX-->>PL: frame or timeout alt timeout PL->>RX: try_recv() non-blocking end else buffer gap note over PL: Do NOT send frame_request_tx PL->>PFQ: drain remaining frames alt exact frame found late PL->>R: render found frame else jump to next available PL->>PL: frame_number = min_buffered else no frames PL->>PL: frame_number += 1 (skip) end end endComments Outside Diff (2)
crates/project/src/configuration.rs, line 1016-1018 (link)"fast"alias silently repurposed — existing recordings may change animation feelThe
"fast"serde alias was previously an alias forMellow(high-damping, tension 470 / mass 3.0 / friction 70). It's now removed fromMellowand instead matched by the newFastvariant (tension 380 / mass 1.0 / friction 30) via exact-string deserialization.Any project JSON on disk that stores
animationStyle: "fast"(from legacy tooling or manual edits) will silently switch from Mellow's parameters to Fast's. The user-visible UI never exposed"fast"as a string value (the old TS type was"slow" | "mellow" | "custom"), so real-world exposure is low — but a defensive serde alias onFastwould make the migration explicit:At minimum, a comment noting that
"fast"is intentionally repurposed rather than a retained alias would help future readers understand the decision.Prompt To Fix With AI
crates/rendering/src/zoom_focus_interpolation.rs, line 1554-1556 (link)_precomputed_cursor,_cursor_smoothing, and_click_springare now stored in the struct but never read. They're passed in through bothnew_arcandnew_arc_with_precomputed_cursor, allocated on every instantiation, and held for the lifetime of the interpolator — but the code that consumed them (interpolate_cursor_at) was removed in this same PR.If these values are genuinely no longer needed, removing them from the struct and call sites cleans up the API and avoids unnecessary
Arcref-count bumps. If they're intentionally parked for a future feature, a// TODO:comment tracking the intent would be helpful so they don't accumulate indefinitely as silenced dead code.The same applies to the corresponding call site in
playback.rswherecursor_smoothingis computed a second time insidebuild_zoom_interpolatorssolely to pass intoZoomFocusInterpolator::new_arc_with_precomputed_cursorwhere it goes unused.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "locks etc" | Re-trigger Greptile